Skip to content

new error code: ASYNC914 unnecessary checkpoints + autofix #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Apr 28, 2023

fixes #70 (merely 5 months after it was opened 😁)

Early WIP, I only got the basic case down.
I started on this before #182, but I'm prioritizing that one.

Remaining TODO:

  • if statements
    • this shouldn't be that complicated, I've mostly got the logic figured out.
  • loops
    • once if is solved this can probably use similar logic, but it'll likely be messy
  • try/except
    • same

@jakkdl
Copy link
Member Author

jakkdl commented Apr 12, 2024

I picked this up and worked on it somewhat recently, but stalled out due to the complexity. The complexity is mostly because it's hard to resist solving the juicy edge cases and autofixing everything, so next time I pick this up I'll just drop the complex parts, see if the basic cases that are working are worth having as a rule, and clean up the PR so it can be merged.

@jakkdl jakkdl force-pushed the trio912_unnecessary_checkpoint branch from 5e44a59 to 358df6d Compare August 18, 2025 15:26
@jakkdl jakkdl changed the title new error code: TRIO912 unnecessary checkpoints + autofix new error code: ASYNC914 unnecessary checkpoints + autofix Aug 18, 2025
@jakkdl jakkdl marked this pull request as ready for review August 18, 2025 15:27
@jakkdl
Copy link
Member Author

jakkdl commented Aug 18, 2025

picked this one up to get it done and dusted. I tried to do simple solutions to avoid the complexity trap I previously fell into, but that only led to further problems and in the end I essentially needed to implement the reverse of the logic in 910/911/913.
The main simplification is avoiding any special handling with the ARTIFICIAL_STATEMENT in loops, but I'm not even sure that should be done as having (potentially "redundant") checkpoints in non-infinite loops can be a good idea.

I also replaced a lot of await foo() with await trio.lowlevel.checkpoint() in other ASYNC91X files in order to test them with ASYNC914 as well. This turned up several issues that I fixed, as well as highlight a case where the autofixer now needs two passes. If there's only a conditional checkpoint it won't be marked by ASYNC914 at first pass, but 910/911/913 will insert a conditional checkpoint, at which point the conditional checkpoint is now redundant. I did not bother fixing this.

I also ended up adding asyncio autofixing support to 910/911/913 by having them insert await asyncio.sleep(0), this ended up being very easy and made the testing easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASYNC912: Warn on unnecessary checkpoints added to avoid ASYNC910/911
1 participant